Skip to content

Add support for Stringified NBT objects - #92

Merged
jcxldn merged 15 commits into
masterfrom
feat/string-nbt-helpers
Feb 14, 2021
Merged

Add support for Stringified NBT objects#92
jcxldn merged 15 commits into
masterfrom
feat/string-nbt-helpers

Conversation

@aakatz3

@aakatz3 aakatz3 commented Feb 13, 2021

Copy link
Copy Markdown
Collaborator

Cool new system to make building NBT strings easy and not awful

@aakatz3 aakatz3 self-assigned this Feb 13, 2021
@aakatz3 aakatz3 added the feature New feature or request label Feb 13, 2021
@aakatz3
aakatz3 force-pushed the feat/string-nbt-helpers branch from 57a5e3b to 7591f25 Compare February 13, 2021 06:20
@codecov

codecov Bot commented Feb 13, 2021

Copy link
Copy Markdown

Codecov Report

Merging #92 (c6b71ae) into master (b794f7b) will decrease coverage by 0.07%.
The diff coverage is 16.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #92      +/-   ##
============================================
- Coverage     20.39%   20.31%   -0.08%     
- Complexity      111      133      +22     
============================================
  Files            63       71       +8     
  Lines          2859     3012     +153     
  Branches        381      402      +21     
============================================
+ Hits            583      612      +29     
- Misses         2261     2379     +118     
- Partials         15       21       +6     
Impacted Files Coverage Δ Complexity Δ
...bdogdiner/stickyapi/common/nbt/NbtCompoundTag.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...bdogdiner/stickyapi/common/nbt/NbtJsonAdapter.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../dumbdogdiner/stickyapi/common/nbt/NbtJsonTag.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../dumbdogdiner/stickyapi/common/nbt/NbtListTag.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...umbdogdiner/stickyapi/common/nbt/NbtStringTag.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...dumbdogdiner/stickyapi/common/util/StringUtil.java 96.72% <0.00%> (-1.62%) 29.00 <0.00> (ø)
...mbdogdiner/stickyapi/common/nbt/NbtBooleanTag.java 61.53% <61.53%> (ø) 9.00 <9.00> (?)
...umbdogdiner/stickyapi/common/nbt/NbtNumberTag.java 84.21% <84.21%> (ø) 8.00 <8.00> (?)
...dogdiner/stickyapi/common/nbt/NbtPrimitiveTag.java 100.00% <100.00%> (ø) 1.00 <1.00> (?)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b794f7b...c6b71ae. Read the comment docs.

@aakatz3
aakatz3 force-pushed the feat/string-nbt-helpers branch from 7591f25 to ad30a23 Compare February 13, 2021 06:26

@kokumaji kokumaji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks fine to me, except for the class names and some method names, would suggest reading up on Java's naming conventions (or we should agree on naming rules in the next meeting?).

Waiting on other admins to comment on this PR.

Comment thread src/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtBooleanTag.java Outdated
Comment thread src/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtCompoundTag.java Outdated
Comment thread src/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtCompoundTag.java Outdated
Comment thread src/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtStringTag.java Outdated
Comment thread src/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtTag.java Outdated
Comment thread src/main/java/com/dumbdogdiner/stickyapi/common/nbt/NbtTag.java
@aakatz3

aakatz3 commented Feb 13, 2021

Copy link
Copy Markdown
Collaborator Author

I used toSNBT instead of toString to be explicit that it's converting to stringifies NBT/mojangson, rather than a JSON string or something else. I can change if people thing it would be better

@jcxldn

jcxldn commented Feb 13, 2021

Copy link
Copy Markdown
Member

I used toSNBT instead of toString to be explicit that it's converting to stringifies NBT/mojangson, rather than a JSON string or something else. I can change if people thing it would be better

I wonder if something like toNBTString() would be a better name, should be the best of both worlds?

@aakatz3

aakatz3 commented Feb 13, 2021

Copy link
Copy Markdown
Collaborator Author

Sure! we good for StringNBT then?

@jcxldn

jcxldn commented Feb 13, 2021

Copy link
Copy Markdown
Member

NbtBooleanTag is correct for Google's Java Style Guide which we will be looking to implement, so should we go with that instead of NBTBooleanTag?

src: https://google.github.io/styleguide/javaguide.html#s5.2.2-class-names (UpperCamelCase class naming)

@kaylendog

Copy link
Copy Markdown
Collaborator

Merging on hold until after module refactor.

@jcxldn
jcxldn force-pushed the feat/string-nbt-helpers branch from 1a29f3b to 72fc012 Compare February 13, 2021 23:32

@jcxldn jcxldn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all files.

@jcxldn
jcxldn dismissed kokumaji’s stale review February 13, 2021 23:57

Review has been addressed.

@jcxldn jcxldn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! (will look at github codeql next. done)

@jcxldn
jcxldn merged commit ef3442b into master Feb 14, 2021
@jcxldn
jcxldn deleted the feat/string-nbt-helpers branch February 14, 2021 00:32
@jcxldn jcxldn mentioned this pull request Feb 14, 2021
3 tasks
jcxldn added a commit that referenced this pull request Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants